-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to react 18 rendering pipeline #1057
Update to react 18 rendering pipeline #1057
Conversation
Bumps React version to 18, and includes all the necessary changes that React 18 demands. Does not yet include the switch to the new rendering API. This is just about getting everything to work with React 18.
React 18 brings with it a new and improved rendering method. This commit activates it. It also removes a workaround pertaining to react-hotkeys that is not used anymore anyway?
This pull request has conflicts ☹ |
…rendering-pipeline
This pull request is deployed at test.editor.opencast.org/1057/2023-08-31_10-14-58/ . |
Fixes a bug causing infinite rerenders in the subtitle view. Limiting how often we set video aspect ratios in redux seems to fix the issue.
This pull request is deployed at test.editor.opencast.org/1057/2023-09-01_09-31-28/ . |
react-hotkeys did not work properly anymore with the upgrade to React 18. Furthermore, it has been unmaintained for several years at this point. Therefore, switching to a newer, maintained library seems a necessity. This commit introduces react-hotkeys-hook to take over hotkeys functionality. To a user everything should work like before and everything it should look like nothing changed (except for maybe the order on the overview page, but that never followed any rhyme or reason in the first place).
This pull request has conflicts ☹ |
Obvious show stoppers have been addressed, so this is finally ready for review. |
This pull request is deployed at test.editor.opencast.org/1057/2023-09-04_12-23-20/ . |
…rendering-pipeline
This pull request is deployed at test.editor.opencast.org/1057/2023-09-04_12-35-06/ . |
This pull request has conflicts ☹ |
…rendering-pipeline
This pull request is deployed at test.editor.opencast.org/1057/2023-09-13_12-59-07/ . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got around to reviewing this. Many of those inline comments are just code-style things that you are free to decide what to do with :P
From testing the deployment I noticed a few more things:
- The order of boxes in the shortcut overview changed. Not a big problem, just making sure you are aware.
- ctrl alt d shows the desktop on Ubuntu :P Again, not a fault of this PR, just FYI
- ctrl alt c does not work for me at all, but neither does
editor.opencast.org
. Tested in Chrome and Firefox.- In fact, it seems like no ctrl+alt shortcut works for me. Not the jumping around ones either. Not for the current main deployment either. Uhhhm, is that just my desktop environment?
Studio uses react-hotkeys-hook
too btw, that's good.
instead of switch case for additional type safety
Order was arbitrary before, is still arbitrary now. Thanks for the notice tho :D
Should really get to allowing users to set their own shortcuts. Is this something we want to solve in appkit?
#works-for-me. We had users reporting shortcuts not working in the past, but I could never replicate their issues (with browserstack). If you manage to ever pinpoint what's causing issues for you it might help those other users as well. |
Huff, that sounds like a huge undertaking. I didn't plan on doing that anywhere :D But I see your point. For Studio, I also had troubles finding shortcuts that work for most. And I think you can never find one set of shortcuts that will actually work for everyone. So in a sense yes, making it configurable is the only perfect solution. But huff, again, not sure if we want to invest time into that anytime soon...
I checked at least the Ubuntu global shortcut list and there I couldn't find anything for ctrl+alt+j for example. So huff, no idea. Will ask others to test the shortcuts in this PR. |
This pull request is deployed at test.editor.opencast.org/1057/2023-09-20_13-01-17/ . |
This pull request is deployed at test.editor.opencast.org/1057/2023-09-20_13-11-36/ . |
`react-hotkeys-hook` does not play nice with MacOs. Long story short, we have to use the more reliable `shift` instead of `mod`. Also, we cannot use `option`, so we have to wait with rewriting `alt` as `option` until displaying.
This pull request is deployed at test.editor.opencast.org/1057/2023-09-20_14-50-07/ . |
This pull request has conflicts ☹ |
…rendering-pipeline
This pull request is deployed at test.editor.opencast.org/1057/2023-09-21_10-13-45/ . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All code stuff was resolved. And now the new shortcuts also work for me. With the exception of shift+alt+i which opens a Chrome report window in Chrome.
This pull request has conflicts ☹ |
…rendering-pipeline
This pull request is deployed at test.editor.opencast.org/1057/2023-09-21_12-10-15/ . |
React 18 brings with it a new and improved rendering method. React reminds everyone of this in the console:
This PR updates the editor to that new method, but has not yet fixed all the errors it causes.
Known errors:
All of our global hotkeys do not work anymore, except the timeline ones.The list component in the subtitle editor now causes an endless loop, completely breaking the editor.